feat(endpoint): reject alias property on unsupported record types#6188
Conversation
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
Pull Request Test Coverage Report for Build 22183896879Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
ivankatliarchuk
left a comment
There was a problem hiding this comment.
Could we add tests here as well
So it's clear where the validation applies?
| // TODO: rename to Validate | ||
| // CheckEndpoint Check if endpoint is properly formatted according to RFC standards | ||
| func (e *Endpoint) CheckEndpoint() bool { | ||
| if !e.supportAlias() { |
There was a problem hiding this comment.
| if !e.supportAlias() { | |
| if !e.supportsAlias { |
| // CheckEndpoint Check if endpoint is properly formatted according to RFC standards | ||
| func (e *Endpoint) CheckEndpoint() bool { | ||
| if !e.supportAlias() { | ||
| if _, ok := e.GetBoolProviderSpecificProperty("alias"); ok { |
There was a problem hiding this comment.
Can we have constant with TODO comment to review source/annotations package, as we have annotations defined in location that cause circular dependencies.
| func (e *Endpoint) CheckEndpoint() bool { | ||
| if !e.supportAlias() { | ||
| if _, ok := e.GetBoolProviderSpecificProperty("alias"); ok { | ||
| log.Debugf("Endpoint %s of type %s does not support alias records in ExternalDNS", e.DNSName, e.RecordType) |
There was a problem hiding this comment.
ExternalDNS not required
There was a problem hiding this comment.
It's a warning. Basically this is a misconfiguration on client side
There was a problem hiding this comment.
Is this log in unit tests?
There was a problem hiding this comment.
I've fixed the log message and added unit tests.
| Targets: Targets{"10 5 5060 sip.example.com."}, | ||
| ProviderSpecific: ProviderSpecific{{Name: "alias", Value: "true"}}, | ||
| }, | ||
| expected: false, |
There was a problem hiding this comment.
Could you clarify what you mean by "false is default"?
There was a problem hiding this comment.
You do not need explicit false, as its a default value
| func (e *Endpoint) CheckEndpoint() bool { | ||
| if !e.supportAlias() { | ||
| if _, ok := e.GetBoolProviderSpecificProperty("alias"); ok { | ||
| log.Debugf("Endpoint %s of type %s does not support alias records in ExternalDNS", e.DNSName, e.RecordType) |
There was a problem hiding this comment.
Is this log in unit tests?
…alias constant Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
|
/approve |
|
/approve cancel |
|
|
||
| // TODO: review source/annotations package to consolidate alias key definitions; | ||
| // currently duplicated here to avoid circular dependency. | ||
| const providerSpecificAlias = "alias" |
There was a problem hiding this comment.
most likely too long, but not an issue. You not using this constant in tests
|
User documentation is missing. Worth to find out the right location for this |
Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
…NAME Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>
|
I could not find example of testing this feature end-2-end. Could you share it pls? |
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| logger, hook := test.NewNullLogger() |
There was a problem hiding this comment.
make sure to use shared construct. let's not create for every test a solution
There was a problem hiding this comment.
Currently, it’s difficult to use a shared construct due to circular dependencies.
Once this PR is merged, I was planning to refactor the structure and introduce a shared construct in a follow-up PR.
|
Yes. I verified this behavior end-to-end using the DNSEndpoint CRD. I was able to successfully create a normal MX record using the following manifest: apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
name: alias
namespace: default
spec:
endpoints:
- dnsName: test.mail.example.com
recordType: MX
recordTTL: 600
targets:
- "10 mail.example.com"However, when I added the alias apiVersion: externaldns.k8s.io/v1alpha1
kind: DNSEndpoint
metadata:
name: alias
namespace: default
spec:
endpoints:
- dnsName: test.mail.example.com
recordType: MX
recordTTL: 600
targets:
- "10 mail.example.com"
providerSpecific:
- name: "alias"
value: "true"ExternalDNS produced the following warnings and skipped the endpoint: This confirms that MX records with |
|
/approve |
|
I was testing something. And found this log We don't have any aliases set |
|
needs clarification. as was not there before /remove-approve |
|
This behavior is not introduced by this PR and already exists in the current master branch. To verify, I switched to the latest |
|
The logic most likely had changed in this PR https://github.com/kubernetes-sigs/external-dns/pull/6021/changes I'll try to understand it better over a weekend. |
|
Tested against https://github.com/kubernetes-sigs/external-dns/tree/v0.20.0. The message
Is there as well /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
…_total * master: (21 commits) refactor(testutils): extract log test helpers into subpackage to fix (kubernetes-sigs#6236) chore(deps): bump mkdocs-material (kubernetes-sigs#6237) feat(endpoint): reject alias property on unsupported record types (kubernetes-sigs#6188) fix(charts): Skip cluster-scope RBAC on namespaced (kubernetes-sigs#5843) chore(deps): bump the dev-dependencies group across 1 directory with 3 updates (kubernetes-sigs#6226) feat(pdns): add --[no-]prefer-alias flag and alias annotation support (kubernetes-sigs#6129) fix(ci): failed to download the coveralls binary from GitHub releases (kubernetes-sigs#6228) docs: add external-dns-pscloud-webhook to New providers list (kubernetes-sigs#6214) fix(crd): allow trailing dot in CNAME targets (kubernetes-sigs#6218) docs: added deep wiki badge (kubernetes-sigs#6215) feat(crd): Support MX record with trailing dot (kubernetes-sigs#6163) chore(source): standardize sources with merge endpionts and deduplicate targets (kubernetes-sigs#6174) chore(store): Added RESTConfig() to ClientGenerator (kubernetes-sigs#6177) chore(ingress): clarify that both IP and Hostname are collected from LoadBalancer status (kubernetes-sigs#6138) chore(endpoint): added empty checks (kubernetes-sigs#6157) chore(linter): enable unparam (kubernetes-sigs#6160) fix(tlsutils): fix nil error wrapping and wrong env var in TLS config (kubernetes-sigs#6198) chore(endpoint): harden crypto (kubernetes-sigs#6197) feat(fqdn): Deduplicate and sort ExecTemplate output. Add functions (kubernetes-sigs#6173) benchmark(endpoint): endpoint benchmarks (kubernetes-sigs#6156) ...
* master: (23 commits) refactor(testutils): extract log test helpers into subpackage to fix (kubernetes-sigs#6236) chore(deps): bump mkdocs-material (kubernetes-sigs#6237) feat(endpoint): reject alias property on unsupported record types (kubernetes-sigs#6188) fix(charts): Skip cluster-scope RBAC on namespaced (kubernetes-sigs#5843) chore(deps): bump the dev-dependencies group across 1 directory with 3 updates (kubernetes-sigs#6226) feat(pdns): add --[no-]prefer-alias flag and alias annotation support (kubernetes-sigs#6129) fix(ci): failed to download the coveralls binary from GitHub releases (kubernetes-sigs#6228) docs: add external-dns-pscloud-webhook to New providers list (kubernetes-sigs#6214) fix(crd): allow trailing dot in CNAME targets (kubernetes-sigs#6218) docs: added deep wiki badge (kubernetes-sigs#6215) feat(crd): Support MX record with trailing dot (kubernetes-sigs#6163) chore(source): standardize sources with merge endpionts and deduplicate targets (kubernetes-sigs#6174) chore(store): Added RESTConfig() to ClientGenerator (kubernetes-sigs#6177) chore(ingress): clarify that both IP and Hostname are collected from LoadBalancer status (kubernetes-sigs#6138) chore(endpoint): added empty checks (kubernetes-sigs#6157) chore(linter): enable unparam (kubernetes-sigs#6160) fix(tlsutils): fix nil error wrapping and wrong env var in TLS config (kubernetes-sigs#6198) chore(endpoint): harden crypto (kubernetes-sigs#6197) feat(fqdn): Deduplicate and sort ExecTemplate output. Add functions (kubernetes-sigs#6173) benchmark(endpoint): endpoint benchmarks (kubernetes-sigs#6156) ...
What does it do ?
This PR adds validation logic to
CheckEndpointto ensure thatalias=trueis only allowed for supported record types.Endpoints with unsupported combinations (e.g., MX + alias=true) are now rejected during validation instead of being handled silently in providers.
Motivation
Currently, invalid alias configurations can pass through the pipeline and be handled (or silently fixed) inside specific providers, which leads to inconsistent behavior and makes user mistakes harder to detect.
Related issues: #6017
More